Fix sketch-on-face sketches to be in the artifact graph #5489
Conversation
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| for exec_artifact in exec_artifacts.values() { | ||
| merge_artifact_into_map(&mut map, exec_artifact.clone()); | ||
| } |
There was a problem hiding this comment.
This is the core of the change.
It's unfortunate that there are two loops, this one and the one above. I think this might cause some problems when there are multiple sketches-on-face/place on the same face/plane.
jtran
force-pushed
the
jtran/plane-code-ref
branch
from
0f00752 to
88b06d1
Compare
jtran
force-pushed
the
jtran/plane-code-ref
branch
from
88b06d1 to
1d627a4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5489 +/- ##
==========================================
+ Coverage 86.26% 86.35% +0.08%
==========================================
Files 95 95
Lines 35901 35950 +49
==========================================
+ Hits 30970 31044 +74
+ Misses 4931 4906 -25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Leveraging the new data that was introduced by #5489
There was a problem hiding this comment.
This is working great for me! I actually was already able to stack a little PR off of your branch #5497
Part of #5385.
We generate artifacts in
sketch.rspurely client-side, and we return those inexecArtifacts. It's super special for sketch on face and sketch on plane because it was a workaround for when building the artifact graph required an engine command. We use the information in those artifacts in certain cases, but it turns out that we never actually added those to the graph itself.The main change in this PR is to add those client-side-only artifacts to the graph. See the second commit.
There's more rejiggering to make sketches-on-{face or plane} less special.
This is the new node in the graph for
src/wasm-lib/kcl/tests/artifact_graph_example_code1/input.kcl. See the issue #5385 (comment) for more discussion.The thing that jumps out at me is that the profile doesn't depend on the sketch-on-face. You also can't traverse from the profile to the sketch-on-face because there's no graph edge from the wall to the sketch-on-face.